-
Notifications
You must be signed in to change notification settings - Fork 227
fix: patch dataloader to propagate context to new fiber #1760
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: patch dataloader to propagate context to new fiber #1760
Conversation
| end | ||
|
|
||
| def patch | ||
| return if gem_version < Gem::Version.new('2.1.8') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The target of the patch, Dataloader#spawn_fiber method existed from version 2.1.8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we would have some automated tests that test this functionality to avoid regressions.
@rmosolgo do you have any recommendations on how to set up a test to cover this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had added a test for the expected behavior of #spawn_fiber.
I also confirmed that it failed without the patch.
|
cc: @rmosolgo Request for review |
|
LGTM 👍 |
5c864f9 to
9108622
Compare
I think this should fix #127